Skip to content

fix: security audit — plugin RCE, CORS, token leak, shell exec, login rate limit - #1106

Open
wjc2821296948 wants to merge 8 commits into
siteboon:mainfrom
wjc2821296948:fix/security-audit-pr
Open

fix: security audit — plugin RCE, CORS, token leak, shell exec, login rate limit#1106
wjc2821296948 wants to merge 8 commits into
siteboon:mainfrom
wjc2821296948:fix/security-audit-pr

Conversation

@wjc2821296948

@wjc2821296948 wjc2821296948 commented Aug 5, 2026

Copy link
Copy Markdown

Summary

This PR fixes five security issues found by a manual security audit of the CloudCLI server codebase. Each commit is a self-contained fix and ships with its own PR comment below describing the issue, the failure scenario, and the resolution.

Findings

P0 — Plugin install executes npm run build on attacker-controlled repositories

server/modules/plugins/plugin-registry.service.ts cloned an arbitrary Git URL into a temp directory and ran npm run build whenever package.json declared a build script. --ignore-scripts blocks postinstall hooks but does not cover npm run build. Any party able to supply a plugin URL — including a user tricked into pasting one, or a leaked auth token — gained remote code execution on the CloudCLI host.

Fix: the build script now requires explicit opt-in (allowBuild: true) on the install/update call, after the operator has manually inspected the script. The HTTP routes accept an allowBuild field in the JSON body. A process-wide escape hatch (setAllowPluginBuildScript) is exposed for tests. See fix(plugins): disable auto-running npm run build during plugin install.

P1 — GitHub personal access token leaks via the clone-progress SSE stream

server/modules/projects/services/project-clone.service.ts embedded the supplied GitHub PAT into the clone URL (https://<token>@host/...) and forwarded git's stdout/stderr verbatim to the SSE clone-progress feed. git echoes the full URL in progress output, so the token leaked through every progress event.

Fix: run every stdout/stderr line through sanitizeGitError before relaying as progress. The function already replaces the token string with ***; the only behavior change is that the sanitized text is what the SSE consumer sees during the clone (and not only after the clone fails). See fix(projects): sanitize GitHub tokens from clone progress stream.

P1 — CORS reflects any Origin header

app.use(cors({ exposedHeaders: [...] })) was invoked with no origin option, so the cors package reflected the request's Origin header back unchanged in Access-Control-Allow-Origin for every cross-origin request. Combined with the fact that /api routes are protected by a bearer JWT that the client keeps in localStorage, any malicious site a victim visits could read responses from the server on the victim's behalf.

Fix: replace the default reflector with a callback that only allows the origin through when its host:port matches the server's own host:port. Same-origin requests (no Origin header) continue to be allowed through. Wildcard binds (0.0.0.0/::) accept any host on the configured port, preserving the LAN-hosted use case while still refusing unrelated public origins. See fix(server): restrict CORS to same host:port as the server.

P2 — System update spawns commands through sh -c

server/modules/system/system.module.ts invoked spawn('sh', ['-c', commandString], ...) for the in-app update workflow. The current templates are all literals, but the call shape is a footgun: any future change that splices appRoot, homeDirectory, an environment variable, or any operator-controlled string into the template becomes a classic shell command injection. A poisoned $PATH already substitutes a malicious npm/git binary.

Fix: split the executor into (command, args) argv arrays with shell: false. The git workflow legitimately chains three commands, so it still uses sh -c with a fully literal argument string — every other path now spawns the executable directly with no shell at all. Tests are updated to match the new argv signature. See fix(system): spawn update commands without a shell.

P2 — /api/auth/login and /api/auth/register have no rate limiting

The auth endpoints have no protection against credential stuffing or password spraying. An attacker who can reach the server (default bind 0.0.0.0:3001) can run an unbounded number of guesses per second from a single IP.

Fix: add a per-client sliding-window rate limiter (server/modules/auth/rate-limit.middleware.ts) that defaults to 10 attempts per minute and a 60-second lockout window once the cap is hit. The limiter keys on the TCP peer address (or the first X-Forwarded-For entry when behind a reverse proxy), so it scales to single-user self-hosted installs without needing a shared store. Successful and failed attempts both consume a slot; the limiter does not let a misbehaving client extend a lockout by retrying. Covered by a focused unit test. See fix(auth): rate-limit login and registration per client.

Test plan

  • npm run typecheck (pending — repo has no node_modules in this checkout)
  • npm run test
  • npm run build

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Security

    • Improved CORS validation and added rate limiting to authentication endpoints.
    • Plugin builds remain disabled unless explicitly enabled.
    • Git progress and error messages now redact exposed credentials.
    • Reduced shell command interpretation risks during system updates.
  • Bug Fixes

    • Failed plugin updates now restore previously running plugins.
    • Improved plugin update safety by validating changes before applying them.
  • Tests

    • Added coverage for rate limiting, credential redaction, plugin recovery, and system update handling.

wjc2821296948 and others added 5 commits August 5, 2026 15:14
`installPluginFromGit` and `updatePluginFromGit` cloned a remote Git repository
and ran `npm run build` whenever the package.json declared a build script.
Build scripts execute arbitrary code with the server process's privileges, so
any party able to supply a plugin URL (e.g. an authenticated user tricked into
pasting a malicious URL, or a compromised auth token) gained remote code
execution on the CloudCLI host.

The build script is now opt-in: the caller must pass `allowBuild: true` to the
install/update service after manually inspecting the build command. The HTTP
`POST /api/plugins/install` and `POST /api/plugins/<name>/update` endpoints
accept an explicit `allowBuild: true` in the JSON body for that purpose. A
process-wide escape hatch (`setAllowPluginBuildScript`) is exposed for tests.

Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
`startCloneProject` embeds the user-supplied GitHub personal access token into
the clone URL (https://<token>@host/...) and streams `git`'s stdout/stderr
straight into the SSE `clone-progress` feed via `onProgress`. `git` echoes the
full clone URL in its progress output, so every progress event leaks the
token to whoever is watching the feed (which includes the user, but is also
captured in any server-side logs that subscribe to the same stream).

Run every stdout/stderr line through `sanitizeGitError` before relaying as
progress. The function already replaces the token string with `***`; the only
behavior change is that the sanitized text is what the SSE consumer sees
during the clone (and not only after the clone fails).

Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
`app.use(cors({ exposedHeaders: [...] }))` invoked the `cors` package with no
`origin` option, so the package reflected the request's `Origin` header back
unchanged in `Access-Control-Allow-Origin` for every cross-origin request.
Combined with the fact that most `/api` routes are only protected by a
bearer JWT that the client keeps in localStorage, any malicious site a
victim visits in the same browser could read responses from the server on
the victim's behalf by issuing requests with the victim's token.

Replace the default reflector with a callback that only allows the origin
through when its host:port matches the server's own host:port. Same-origin
requests (no Origin header) continue to be allowed through. Wildcard binds
(0.0.0.0/::) accept any host on the configured port, which preserves the
LAN-hosted use case while still refusing unrelated public origins.

Move `SERVER_PORT` / `HOST` / `DISPLAY_HOST` / `VITE_PORT` declarations above
the CORS middleware so the reflector can read them at module load time.

Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
`runShellCommand` invoked `spawn('sh', ['-c', commandString], ...)`, passing
the entire command as a single shell string. The current templates are all
literals, but the call shape is a footgun: any future change that splices
`appRoot`, `homeDirectory`, an environment variable, or any operator-controlled
string into the template becomes a classic shell command injection, with the
server process's privileges. A poisoned `$PATH` would already be enough to
substitute a malicious `npm`/`git` binary into the call.

Split the executor into (command, args) argv arrays and disable the shell.
The git workflow still legitimately chains three commands, so it falls back
to `sh -c` with a fully literal argument string (no string concatenation
with external values) — every other path now spawns the executable
directly with `shell: false`.

Update the service to plan each branch as `{ command, args }` and update the
existing service tests to match the new argv signature.

Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
The `/api/auth/login` and `/api/auth/register` endpoints have no protection
against credential stuffing or password spraying. An attacker who can reach
the server (default bind `0.0.0.0:3001`) can run an unbounded number of
guesses per second from a single IP. Bcrypt with 12 rounds makes each guess
slow but does not make online brute force infeasible — over a long enough
window any 8-character password falls.

Add a per-client sliding-window rate limiter that defaults to 10 attempts
per minute and a 60-second lockout window once the cap is hit. The limiter
keys on the TCP peer address (or the first `X-Forwarded-For` entry when
behind a reverse proxy), so it scales to single-user self-hosted installs
without needing a shared store. Successful and failed attempts both
consume a slot; the limiter does not let a misbehaving client extend a
lockout by retrying.

Cover the limiter with a focused unit test that verifies the under-cap,
over-cap, lockout-no-extend, rolling-window, and per-client-key behaviors.

Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The server now validates CORS origins, rate-limits authentication, controls plugin build scripts and update replacement, redacts Git clone output, and executes system update commands with separate arguments.

Changes

Server origin validation

Layer / File(s) Summary
Configured CORS origin validation
server/index.ts
Startup constants define server hosts and ports. CORS allows matching origins and requests without an Origin header while rejecting malformed or mismatched origins.

Authentication rate limiting

Layer / File(s) Summary
Authentication limiter and validation
server/modules/auth/rate-limit.middleware.ts, server/modules/auth/auth.routes.ts, server/modules/auth/tests/rate-limit.middleware.test.ts
Registration and login share a per-client sliding-window limiter. Tests cover lockouts, retry headers, expiration, and client isolation.

Plugin build policy

Layer / File(s) Summary
Explicit plugin build permission
server/modules/plugins/plugin-registry.service.ts, server/modules/plugins/plugins.routes.ts, server/modules/plugins/plugins.service.ts
Plugin builds are disabled by default. Install and update routes accept allowBuild: true and forward the option through the service layers.
Validated plugin update replacement
server/modules/plugins/plugin-registry.service.ts, server/modules/plugins/plugins.service.ts, server/modules/plugins/tests/plugins.service.test.ts
Updates use a temporary clone and replace the live plugin only after validation and installation succeed. Failed updates clean up temporary files and restart the previous running server.

Clone output sanitization

Layer / File(s) Summary
Chunk-safe clone output redaction
server/modules/projects/services/project-clone.service.ts, server/modules/projects/tests/project-clone.service.test.ts
Stdout and stderr redactors preserve token fragments across chunks and flush buffered output when streams close.

System command execution

Layer / File(s) Summary
Direct command execution contract
server/modules/system/system.module.ts, server/modules/system/system.service.ts, server/modules/system/tests/system.service.test.ts
System updates pass executables and argument arrays separately. The command runner uses spawn with shell: false. Tests verify Git, npm, and platform invocations.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AuthRoutes
  participant RateLimiter
  participant AuthHandler
  Client->>AuthRoutes: Send registration or login request
  AuthRoutes->>RateLimiter: Check client attempt
  RateLimiter->>AuthHandler: Invoke next handler when allowed
  RateLimiter-->>Client: Return 429 with Retry-After when locked out
Loading

Possibly related PRs

Suggested reviewers: blackmammoth

Poem

A rabbit checks each origin bright,
And guards the login gate at night.
Plugins build by granted sign,
Git hides tokens line by line.
Commands hop with arguments free—
Safe server paths for you and me.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the PR's primary security fixes, including plugin RCE, CORS, token leaks, shell execution, and login rate limiting.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/modules/auth/rate-limit.middleware.ts`:
- Around line 93-95: Update the lockout calculation in the rate-limit handling
around record.timestamps and blockedUntil so blockedUntil is at least the
earliest retained timestamp plus windowMs, while preserving the existing
lockoutMs-based delay when it is later. Keep retryAfterSeconds derived from the
final blockedUntil value so Retry-After reflects the next permitted request.

In `@server/modules/auth/tests/rate-limit.middleware.test.ts`:
- Around line 115-122: Update the timing assertion in the rate-limit test around
limiter.middleware to advance time beyond one second instead of 500 ms, then
assert the resulting Retry-After header reflects the preserved lockout rather
than a reset two-second window. Keep the existing 429 status assertion and
verify the updated header value discriminates between the two behaviors.

In `@server/modules/plugins/plugins.service.ts`:
- Around line 116-121: Update update() so dependencies.update() stages and
validates the candidate before modifying the live plugin directory or stopping a
running server; only after successful validation should the current plugin be
replaced and restarted as needed. Ensure a rejected build leaves both the live
directory and running server unchanged, and add coverage for a running plugin
with a build script updated without allowBuild: true.

In `@server/modules/projects/services/project-clone.service.ts`:
- Around line 246-251: Update the clone progress handlers around
sanitizeGitError so credential redaction remains effective when stdout or stderr
data chunks split a token across events. Maintain per-stream carry-over state,
redact only complete available content while retaining a possible token prefix,
and flush any remaining buffered content when each stream closes; add a
regression test that emits a token across two data events and verifies the
concatenated SSE progress output contains no credential fragments.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 18e2a222-ac6b-4c46-b194-7483369fbafe

📥 Commits

Reviewing files that changed from the base of the PR and between f0dca2d and 03e6876.

📒 Files selected for processing (11)
  • server/index.ts
  • server/modules/auth/auth.routes.ts
  • server/modules/auth/rate-limit.middleware.ts
  • server/modules/auth/tests/rate-limit.middleware.test.ts
  • server/modules/plugins/plugin-registry.service.ts
  • server/modules/plugins/plugins.routes.ts
  • server/modules/plugins/plugins.service.ts
  • server/modules/projects/services/project-clone.service.ts
  • server/modules/system/system.module.ts
  • server/modules/system/system.service.ts
  • server/modules/system/tests/system.service.test.ts

Comment thread server/modules/auth/rate-limit.middleware.ts Outdated
Comment thread server/modules/auth/tests/rate-limit.middleware.test.ts Outdated
Comment thread server/modules/plugins/plugins.service.ts Outdated
Comment thread server/modules/projects/services/project-clone.service.ts Outdated
@wjc2821296948

Copy link
Copy Markdown
Author

P0 — Plugin install executes npm run build on attacker-controlled repositories

Vulnerability description

installPluginFromGit(url) and updatePluginFromGit(name) in server/modules/plugins/plugin-registry.service.ts cloned an arbitrary Git URL into a temp directory and ran npm run build whenever the cloned package.json declared a build script. --ignore-scripts blocks postinstall hooks but does not cover npm run build, so the build step ran with the full privileges of the host Node process.

// server/modules/plugins/plugin-registry.service.ts (before)
// runBuildIfNeeded checked only for the existence of a build script,
// not for who authored it or what it contained.
const buildProcess = spawn('npm', ['run', 'build'], {
  cwd: dir,
  stdio: ['ignore', 'pipe', 'pipe'],
});

Any party able to supply a plugin URL — including an authenticated user tricked into pasting a hostile URL, or a leaked auth token — gained remote code execution on the CloudCLI host.

Fix approach

The build script is now opt-in. The runBuildIfNeeded helper takes a new options.allowBuild flag and a process-wide override (setAllowPluginBuildScript); if both are unset, a present build script causes the install/update to fail with a clear error. The HTTP routes POST /api/plugins/install and POST /api/plugins/<name>/update accept an explicit allowBuild: true in the JSON body for callers that have manually vetted the plugin's build command.

// server/modules/plugins/plugin-registry.service.ts (after)
if (!ALLOW_PLUGIN_BUILD_SCRIPT && !options?.allowBuild) {
  return onError(new Error(
    'Plugin declares a "build" script but plugin builds are disabled by default. ' +
    'Plugin build scripts run arbitrary code with the server process privileges. ' +
    'To install this plugin, ship a pre-built artifact and remove the build script, ' +
    'or set `allowBuild: true` after manually inspecting the build script.',
  ));
}
// server/modules/plugins/plugins.routes.ts (after)
router.post('/install', respond((req) => {
  const body = (req.body ?? {}) as { url?: unknown; allowBuild?: unknown };
  // Build scripts run arbitrary code with server privileges. Only opt in
  // when the operator has manually vetted the plugin's build script.
  const allowBuild = body.allowBuild === true;
  return service.install(body.url, { allowBuild });
}));

Referenced code

  • Commit: 55d126efix(plugins): disable auto-running npm run build during plugin install
  • Files: server/modules/plugins/plugin-registry.service.ts, server/modules/plugins/plugins.service.ts, server/modules/plugins/plugins.routes.ts

@wjc2821296948

Copy link
Copy Markdown
Author

P1 — GitHub personal access token leaks via the clone-progress SSE stream

Vulnerability description

startCloneProject in server/modules/projects/services/project-clone.service.ts embedded the user-supplied GitHub personal access token into the clone URL (https://<token>@host/...) and forwarded git's stdout/stderr verbatim to the SSE clone-progress feed via onProgress. git echoes the full clone URL in its progress output, so every progress event leaked the token to whoever was watching the feed — including the user, but also anyone whose logs subscribe to the same stream.

// server/modules/projects/services/project-clone.service.ts (before)
gitProcess.stdout?.on('data', (data: Buffer | string) => {
  const message = data.toString().trim();
  if (message) {
    handlers.onProgress(message);  // ← unfiltered, may contain https://<token>@host/...
  }
});

gitProcess.stderr?.on('data', (data: Buffer | string) => {
  const message = data.toString().trim();
  lastError = message;
  if (message) {
    handlers.onProgress(message);  // ← same leak on stderr
  }
});

Note: sanitizeGitError was already applied to lastError (line 280) — but after the same text had already been forwarded to the SSE consumer as a progress event.

Fix approach

Run every stdout/stderr line through sanitizeGitError before relaying as progress. The function already replaces the token string with ***; the only behavior change is that the sanitized text is what the SSE consumer sees during the clone (and not only after the clone fails).

// server/modules/projects/services/project-clone.service.ts (after)
gitProcess.stdout?.on('data', (data: Buffer | string) => {
  const message = data.toString().trim();
  if (!message) return;
  // `git` echoes the clone URL (with the embedded auth token) in progress
  // messages. Always sanitize before forwarding to the SSE stream so the
  // token is not exposed to anyone watching the clone-progress feed.
  handlers.onProgress(sanitizeGitError(message, githubToken));
});

gitProcess.stderr?.on('data', (data: Buffer | string) => {
  const message = data.toString().trim();
  lastError = message;
  if (!message) return;
  // Same token-leak risk on stderr. Sanitize before relaying as progress.
  handlers.onProgress(sanitizeGitError(message, githubToken));
});

Referenced code

  • Commit: 15dbf2bfix(projects): sanitize GitHub tokens from clone progress stream
  • File: server/modules/projects/services/project-clone.service.ts (lines 244-259)

@wjc2821296948

Copy link
Copy Markdown
Author

P1 — CORS reflects any Origin header

Vulnerability description

app.use(cors({ exposedHeaders: [...] })) in server/index.ts was invoked with no origin option, so the cors package reflected the request's Origin header back unchanged in Access-Control-Allow-Origin for every cross-origin request. Combined with the fact that most /api routes are only protected by a bearer JWT that the client keeps in localStorage (and which the frontend attaches to every fetch via authenticatedFetch), any malicious site a victim visits in the same browser session could read responses from the server on the victim's behalf by issuing requests with the victim's token.

// server/index.ts (before)
// Reflects any Origin header back. With JSON + bearer-token auth this is
// safe against classic CSRF (the attacker cannot read the response) but
// fails open once any XSS lands in the host page, and it makes every
// authenticated endpoint reachable from any origin in the browser.
app.use(cors({ exposedHeaders: ['X-Refreshed-Token', 'X-Auth-Error'] }));

Fix approach

Replace the default reflector with a callback that only allows the origin through when its host:port matches the server's own host:port. Same-origin requests (no Origin header — e.g. server-to-server, curl, the Electron desktop app) continue to be allowed through. Wildcard binds (0.0.0.0/::) accept any host on the configured port, which preserves the LAN-hosted use case while still refusing unrelated public origins.

// server/index.ts (after)
const corsOriginReflector = (
  origin: string | undefined,
  callback: (err: Error | null, allow?: boolean) => void,
) => {
  // No Origin header → same-origin request (e.g. server-to-server, curl);
  // these are not subject to CORS and should always be allowed through.
  if (!origin) {
    callback(null, true);
    return;
  }

  try {
    const parsed = new URL(origin);
    const requestHost = parsed.hostname;
    const requestPort = parsed.port || (parsed.protocol === 'https:' ? '443' : '80');
    const serverHost = HOST === '0.0.0.0' || HOST === '::' ? requestHost : HOST;
    const serverPort = String(SERVER_PORT);

    if (requestHost === serverHost && requestPort === serverPort) {
      callback(null, true);
      return;
    }
  } catch {
    // Malformed Origin header — refuse.
  }

  callback(null, false);
};

app.use(cors({
  origin: corsOriginReflector,
  exposedHeaders: ['X-Refreshed-Token', 'X-Auth-Error'],
}));

SERVER_PORT / HOST / DISPLAY_HOST / VITE_PORT are moved above the CORS middleware so the reflector can read them at module load time.

Referenced code

  • Commit: 990ede3fix(server): restrict CORS to same host:port as the server
  • File: server/index.ts (CORS section, plus the SERVER_PORT/HOST move)

@wjc2821296948

Copy link
Copy Markdown
Author

P2 — System update spawns commands through sh -c

Vulnerability description

runShellCommand in server/modules/system/system.module.ts invoked spawn('sh', ['-c', commandString], ...), passing the entire command as a single shell string. The current templates are all literals, but the call shape is a footgun: any future change that splices appRoot, homeDirectory, an environment variable, or any operator-controlled string into the template becomes a classic shell command injection, executed with the server process's privileges. A poisoned $PATH is already enough to substitute a malicious npm/git binary into the call.

// server/modules/system/system.module.ts (before)
function runShellCommand(
  command: string,
  workingDirectory: string,
  environment: NodeJS.ProcessEnv,
  onOutput: (output: string) => void,
  onErrorOutput: (errorOutput: string) => void,
): Promise<...> {
  return new Promise((resolve, reject) => {
    const childProcess = spawn('sh', ['-c', command], {
      cwd: workingDirectory,
      env: environment,
    });
    ...

Fix approach

Split the executor into (command, args) argv arrays and disable the shell. The git workflow legitimately chains three commands, so it falls back to sh -c with a fully literal argument string (no string concatenation with external values) — every other path now spawns the executable directly with shell: false.

// server/modules/system/system.module.ts (after)
function runShellCommand(
  command: string,
  args: string[],
  workingDirectory: string,
  environment: NodeJS.ProcessEnv,
  onOutput: (output: string) => void,
  onErrorOutput: (errorOutput: string) => void,
): Promise<...> {
  return new Promise((resolve, reject) => {
    const childProcess = spawn(command, args, {
      cwd: workingDirectory,
      env: environment,
      shell: false,
    });
    ...

The service now plans each branch as { command, args }:

// server/modules/system/system.service.ts (after)
const updatePlan = dependencies.isPlatform
  ? { command: 'npm', args: ['run', 'update:platform'] }
  : dependencies.installMode === 'git'
    ? { command: 'sh', args: ['-c', 'git checkout main && git pull && npm install'] }
    : { command: 'npm', args: ['install', '-g', '@cloudcli-ai/cloudcli@latest'] };

Existing service tests are updated to match the new argv signature.

Referenced code

  • Commit: c5bb982fix(system): spawn update commands without a shell
  • Files: server/modules/system/system.module.ts, server/modules/system/system.service.ts, server/modules/system/tests/system.service.test.ts

@wjc2821296948

Copy link
Copy Markdown
Author

P2 — /api/auth/login and /api/auth/register have no rate limiting

Vulnerability description

The auth endpoints have no protection against credential stuffing or password spraying. An attacker who can reach the server (default bind 0.0.0.0:3001) can run an unbounded number of guesses per second from a single IP. Bcrypt with 12 rounds makes each guess slow but does not make online brute force infeasible — over a long enough window any 8-character password falls.

// server/modules/auth/auth.routes.ts (before)
router.post('/login', async (req, res, next) => {
  try {
    const body = req.body as { username?: unknown; password?: unknown };
    res.json(await service.login(body.username, body.password));
  } catch (error) {
    next(error);
  }
});

Fix approach

Add a per-client sliding-window rate limiter (server/modules/auth/rate-limit.middleware.ts) that defaults to 10 attempts per minute and a 60-second lockout window once the cap is hit. The limiter keys on the TCP peer address (or the first X-Forwarded-For entry when behind a reverse proxy), so it scales to single-user self-hosted installs without needing a shared store. Successful and failed attempts both consume a slot; the limiter does not let a misbehaving client extend a lockout by retrying.

// server/modules/auth/rate-limit.middleware.ts (excerpt)
const middleware: RequestHandler = (req: Request, res: Response, next) => {
  const clientKey = readClientKey(req);
  const now = clock();
  let record = records.get(clientKey);
  if (!record) {
    record = { timestamps: [], blockedUntil: 0 };
    records.set(clientKey, record);
  }

  // An active lockout short-circuits the limiter; do not consume an attempt
  // slot so a misbehaving client cannot extend the lockout indefinitely.
  if (record.blockedUntil > now) {
    const retryAfterSeconds = Math.max(1, Math.ceil((record.blockedUntil - now) / 1000));
    res.setHeader('Retry-After', String(retryAfterSeconds));
    res.status(429).json({ success: false, error: { code: 'RATE_LIMITED', ... } });
    return;
  }

  const cutoff = now - windowMs;
  record.timestamps = record.timestamps.filter((timestamp) => timestamp > cutoff);

  if (record.timestamps.length >= maxAttempts) {
    record.blockedUntil = now + lockoutMs;
    ...res.status(429)...
    return;
  }

  record.timestamps.push(now);
  next();
};

The limiter is mounted on both /register and /login:

// server/modules/auth/auth.routes.ts (after)
router.post('/register', authRateLimit.middleware, async (req, res, next) => { ... });
router.post('/login', authRateLimit.middleware, async (req, res, next) => { ... });

A focused unit test (server/modules/auth/tests/rate-limit.middleware.test.ts) verifies the under-cap, over-cap, lockout-no-extend, rolling-window, and per-client-key behaviors.

Referenced code

  • Commit: 03e6876fix(auth): rate-limit login and registration per client
  • Files: server/modules/auth/auth.routes.ts, server/modules/auth/rate-limit.middleware.ts (new), server/modules/auth/tests/rate-limit.middleware.test.ts (new)

@blackmammoth

Copy link
Copy Markdown
Member

hey @wjc2821296948, can you check the coderabbit comments?

@blackmammoth
blackmammoth marked this pull request as draft August 5, 2026 15:59
@wjc2821296948

Copy link
Copy Markdown
Author

hey @wjc2821296948, can you check the coderabbit comments?

OK,I'm checking.

wjc2821296948 and others added 3 commits August 6, 2026 02:46
…uest

When `lockoutMs` is shorter than `windowMs`, the previous lockout
calculation set `blockedUntil = now + lockoutMs`, but the rolling window
retained `maxAttempts` timestamps whose earliest expiry was
`timestamps[0] + windowMs`. The client was told to retry in `lockoutMs`
seconds, but on its next request the limiter tripped again — starting a
new lockout — because the cap was still full. This produced a confusing
back-off pattern in which the client could never make forward progress
without burning another lockout cycle.

Set `blockedUntil` to `max(now + lockoutMs, earliestRetainedTimestamp +
windowMs)` so `Retry-After` always points at the next moment a request can
succeed. `retryAfterSeconds` is now derived from the final `blockedUntil`
value, keeping the header consistent with the body.

The "lockout does not extend" test previously advanced the clock by 500
ms — still inside both the original lockout and the rolling window — so
the assertion could not discriminate between a preserved lockout and a
newly reset one. Advance by 1.1 s instead and assert the updated
`Retry-After` header reflects the remaining lockout time.

Add a focused regression test that configures `lockoutMs < windowMs` and
verifies `Retry-After` returns the rolling-window expiry, not the lockout
length.

Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
`sanitizeGitError` replaces exact matches of the token, but `git`'s
`data` events are arbitrary byte slices, not full messages. A
credential can be split across two consecutive events — for example
`https://ghp_abc...` in one chunk and `...def@github.com/...` in the
next — in which case neither half matches the full token and both
fragments leak through the SSE `clone-progress` feed.

Wrap `sanitizeGitError` in a streaming redactor that buffers up to
`token.length - 1` characters across chunks. Only the safe prefix
(everything older than the last possible token-prefix window) is
forwarded as progress; the trailing window is retained until the next
chunk confirms whether it completes a token. `flush()` is wired to the
`end` event of both streams so a half-token that straddles EOF is
also redacted wholesale.

Add a regression test that splits a token across two stdout chunks and
two stderr chunks and verifies no token fragment reaches the progress
callback.

Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
…ve plugin untouched

`updatePluginFromGit` performed `git pull --ff-only` directly against the
live plugin directory. After my previous commit made `runBuildIfNeeded`
reject updates whose `package.json` declares a build script without
`allowBuild: true`, that rejection now happened *after* the pull had
already mutated the live directory (and after `npm install
--ignore-scripts` had already rewritten `node_modules`). The caller
(`plugins.service.ts update()`) had also already stopped the running
plugin server before invoking the registry. A rejected update therefore
left the operator with both a half-updated plugin directory and a
stopped plugin server.

Switch the registry to the same staging pattern `installPluginFromGit`
already uses: re-clone the plugin's remote URL into a sibling temp
directory, validate the manifest, run `npm install`, apply the build
policy, and only then atomically rename the temp directory over the
live one. A rejection at any step cleans up the temp directory and the
live plugin directory is never touched.

Update the service to restart the previously running plugin server when
the update is rejected — the live directory is unchanged, so a clean
restart restores the previous plugin state.

Cover the new contract with a service test that verifies a rejected
update stops, attempts the update, and then restarts the previously
running server.

Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
@wjc2821296948

Copy link
Copy Markdown
Author

CodeRabbit follow-up — rate-limit Retry-After must reflect the next permitted request

Vulnerability description

CodeRabbit correctly pointed out that the previous lockout calculation set blockedUntil = now + lockoutMs, but if lockoutMs < windowMs, the rolling window retained maxAttempts timestamps whose earliest expiry was timestamps[0] + windowMs. The client was told to retry in lockoutMs seconds, but on its next request the limiter tripped again — starting a new lockout — because the cap was still full.

// server/modules/auth/rate-limit.middleware.ts (before)
if (record.timestamps.length >= maxAttempts) {
  record.blockedUntil = now + lockoutMs;
  const retryAfterSeconds = Math.max(1, Math.ceil(lockoutMs / 1000));
  ...
}

Fix approach

Set blockedUntil to max(now + lockoutMs, earliestRetainedTimestamp + windowMs) so Retry-After always points at the next moment a request can succeed. retryAfterSeconds is derived from the final blockedUntil value.

// server/modules/auth/rate-limit.middleware.ts (after)
if (record.timestamps.length >= maxAttempts) {
  const earliestExpiry = record.timestamps.length > 0
    ? record.timestamps[0]! + windowMs
    : now + windowMs;
  const nextAvailableAt = Math.max(now + lockoutMs, earliestExpiry);
  record.blockedUntil = nextAvailableAt;
  const retryAfterSeconds = Math.max(1, Math.ceil((nextAvailableAt - now) / 1000));
  ...
}

The previously written "lockout does not extend" test advanced the clock by only 500 ms — still inside both the original lockout and the rolling window — so the assertion could not discriminate between a preserved lockout and a newly reset one. Advance by 1.1 s instead and assert the updated Retry-After header reflects the remaining lockout time.

Add a focused regression test that configures lockoutMs < windowMs and verifies Retry-After returns the rolling-window expiry, not the lockout length.

Referenced code

  • Commit: 1b48532fix(auth): make rate-limit Retry-After reflect the next permitted request
  • Files: server/modules/auth/rate-limit.middleware.ts, server/modules/auth/tests/rate-limit.middleware.test.ts

@wjc2821296948

Copy link
Copy Markdown
Author

CodeRabbit follow-up — redact GitHub tokens split across stdout/stderr chunks

Vulnerability description

sanitizeGitError replaces exact matches of the token, but git's data events are arbitrary byte slices, not full messages. A credential can be split across two consecutive events — e.g. https://ghp_abc... in one chunk and ...def@github.com/... in the next — in which case neither half matches the full token and both fragments leak through the SSE clone-progress feed.

// server/modules/projects/services/project-clone.service.ts (before)
gitProcess.stdout?.on('data', (data: Buffer | string) => {
  const message = data.toString().trim();
  if (!message) return;
  handlers.onProgress(sanitizeGitError(message, githubToken));  // ← per-chunk replace; misses split tokens
});

Fix approach

Wrap sanitizeGitError in a streaming redactor that buffers up to token.length - 1 characters across chunks. Only the safe prefix (everything older than the last possible token-prefix window) is forwarded as progress; the trailing window is retained until the next chunk confirms whether it completes a token. flush() is wired to the end event of both streams so a half-token that straddles EOF is also redacted wholesale.

// server/modules/projects/services/project-clone.service.ts (after)
function createStreamingRedactor(token: string | null) {
  if (!token) {
    return { feed: (chunk) => chunk, flush: () => '' };
  }
  const maxPrefix = token.length - 1;
  let buffer = '';
  return {
    feed(chunk: string): string {
      if (!chunk) return '';
      buffer += chunk;
      if (buffer.length <= maxPrefix) return '';
      const safeEnd = buffer.length - maxPrefix;
      const safeSlice = buffer.slice(0, safeEnd);
      buffer = buffer.slice(safeEnd);
      return sanitizeGitError(safeSlice, token);
    },
    flush(): string {
      if (!buffer) return '';
      const remainder = buffer;
      buffer = '';
      return sanitizeGitError(remainder, token);
    },
  };
}

Add a regression test that splits a token across two stdout chunks and two stderr chunks and verifies no token fragment reaches the progress callback.

Referenced code

  • Commit: 8d83216fix(projects): redact GitHub tokens split across stdout/stderr chunks
  • Files: server/modules/projects/services/project-clone.service.ts, server/modules/projects/tests/project-clone.service.test.ts

@wjc2821296948

Copy link
Copy Markdown
Author

CodeRabbit follow-up — stage plugin updates so a rejected update leaves the live plugin untouched

Vulnerability description

updatePluginFromGit previously performed git pull --ff-only directly against the live plugin directory. After the build-policy change in the previous commit, runBuildIfNeeded rejects updates whose package.json declares a build script without allowBuild: true. That rejection now happens after the pull has already mutated the live directory (and after npm install --ignore-scripts has already rewritten node_modules). The caller (plugins.service.ts update()) had also already stopped the running plugin server before invoking the registry, so a rejected update left the operator with both a half-updated plugin directory and a stopped plugin server.

// server/modules/plugins/plugin-registry.service.ts (before)
export function updatePluginFromGit(name, options) {
  ...
  // Performs side effects directly on the live plugin directory.
  const gitProcess = spawn('git', ['pull', '--ff-only', '--'], {
    cwd: pluginDir, ...
  });
  ...
  npmProcess.on('close', (npmCode) => {
    ...
    runBuildIfNeeded(pluginDir, packageJsonPath, options, ...);  // ← rejection after live-dir mutation
  });
}
// server/modules/plugins/plugins.service.ts (before)
async update(pluginName, options) {
  ...
  const wasRunning = dependencies.isServerRunning(pluginName);
  if (wasRunning) await dependencies.stopServer(pluginName);  // ← stopped even if update is rejected
  const plugin = normalizePluginManifest(await dependencies.update(pluginName, options));
  if (wasRunning) await startServerIfAvailable(plugin);
  return { success: true, plugin };
}

Fix approach

Switch the registry to the same staging pattern installPluginFromGit already uses: re-clone the plugin's remote URL into a sibling temp directory, validate the manifest, run npm install, apply the build policy, and only then atomically rename the temp directory over the live one. A rejection at any step cleans up the temp directory and the live plugin directory is never touched.

// server/modules/plugins/plugin-registry.service.ts (after)
const tempDir = fs.mkdtempSync(path.join(pluginsDir, `.tmp-update-${name}-`));
...
const cloneProcess = spawn('git', ['clone', '--depth', '1', '--', remoteUrl, tempDir], ...);
cloneProcess.on('close', (code) => {
  ...
  // Validate manifest, run npm install, apply build policy — all against tempDir.
  ...
  // Only swap into place when every step has succeeded.
  runBuildIfNeeded(tempDir, packageJsonPath, options, () => finalize(manifest), (err) => { cleanupTemp(); reject(err); });
});

Update the service to restart the previously running plugin server when the update is rejected — the live directory is unchanged, so a clean restart restores the previous plugin state.

// server/modules/plugins/plugins.service.ts (after)
async update(pluginName, options) {
  ...
  const wasRunning = dependencies.isServerRunning(pluginName);
  if (wasRunning) await dependencies.stopServer(pluginName);
  try {
    const plugin = normalizePluginManifest(await dependencies.update(pluginName, options));
    if (wasRunning) await startServerIfAvailable(plugin);
    return { success: true, plugin };
  } catch (error) {
    if (wasRunning) await startServerIfAvailable(this.getManifest(pluginName));
    throw error;
  }
}

Cover the new contract with a service test that verifies a rejected update stops, attempts the update, and then restarts the previously running server.

Referenced code

  • Commit: 6ae7bbdfix(plugins): stage plugin updates so a rejected update leaves the live plugin untouched
  • Files: server/modules/plugins/plugin-registry.service.ts, server/modules/plugins/plugins.service.ts, server/modules/plugins/tests/plugins.service.test.ts

@wjc2821296948
wjc2821296948 marked this pull request as ready for review August 6, 2026 07:44

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/modules/auth/tests/rate-limit.middleware.test.ts`:
- Around line 115-121: Correct the comments immediately above the currentTime +=
1100 statement: state that simulated time advances from 1000 to 2100, remains
within the lockout through 3000, and is 100 ms past the rolling-window timestamp
expiry at 2000. Preserve the test code and clarify that these conditions make
the preserved lockout observable via Retry-After.

In `@server/modules/plugins/plugin-registry.service.ts`:
- Around line 417-429: Update the finalize flow to preserve pluginDir: move the
existing live directory to a sibling backup, move tempDir into pluginDir, and
delete the backup only after replacement succeeds. If the second move fails,
restore the backup to pluginDir before rejecting and clean up the staged
directory without losing the previous plugin. Add a regression test covering
failure of the second move.

In `@server/modules/projects/services/project-clone.service.ts`:
- Around line 318-322: Update the stderr handling in the clone process flow
around resolveCloneFailureMessage so lastError accumulates the redacted output
from every chunk instead of overwriting it with raw stderr. Ensure nonzero
process close passes the complete buffered redacted text through
sanitizeGitError, including tokens split across stderr chunks, and add coverage
for that split-token failure case.
- Around line 110-132: The feed/flush redaction logic must prevent complete
credentials from reaching progress output. Update the stream sanitizer returned
by the relevant clone-progress service method to retain enough trailing data,
detect and redact any full token within that buffer, and only emit a prefix once
the token cannot complete across another chunk; in flush, validate the retained
suffix as an actual token prefix and redact it rather than returning it
unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a97e9de1-41be-495d-965e-7abb1f39eba8

📥 Commits

Reviewing files that changed from the base of the PR and between 03e6876 and 6ae7bbd.

📒 Files selected for processing (7)
  • server/modules/auth/rate-limit.middleware.ts
  • server/modules/auth/tests/rate-limit.middleware.test.ts
  • server/modules/plugins/plugin-registry.service.ts
  • server/modules/plugins/plugins.service.ts
  • server/modules/plugins/tests/plugins.service.test.ts
  • server/modules/projects/services/project-clone.service.ts
  • server/modules/projects/tests/project-clone.service.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/modules/auth/rate-limit.middleware.ts

Comment on lines +115 to +121
// Advance time past one second — past the original lockout boundary — and
// verify the block window does NOT reset/extend (would happen if we kept
// consuming slots). Advancing by 1100 ms puts us 100 ms past the original
// `now + lockoutMs` of 3000 but still inside `timestamps[0] + windowMs`
// (1000 + 1000 = 2000), so the preserved lockout is observable in the
// updated `Retry-After` header.
currentTime += 1100;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the simulated-time explanation.

At Line 121, currentTime advances from 1000 to 2100. The lockout expires at 3000, so the test remains inside the lockout. The rolling-window timestamp expires at 2000, so the test is 100 ms past that expiry. Update Lines 115-120 because the current explanation reverses both conditions.

Proposed fix
-  // Advance time past one second — past the original lockout boundary — and
-  // verify the block window does NOT reset/extend (would happen if we kept
-  // consuming slots). Advancing by 1100 ms puts us 100 ms past the original
-  // `now + lockoutMs` of 3000 but still inside `timestamps[0] + windowMs`
-  // (1000 + 1000 = 2000), so the preserved lockout is observable in the
-  // updated `Retry-After` header.
+  // Advance time by 1100 ms. This is 100 ms past the rolling-window expiry
+  // at 2000 and 900 ms before the original lockout expiry at 3000. A
+  // preserved lockout therefore returns the remaining Retry-After duration.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Advance time past one second — past the original lockout boundary — and
// verify the block window does NOT reset/extend (would happen if we kept
// consuming slots). Advancing by 1100 ms puts us 100 ms past the original
// `now + lockoutMs` of 3000 but still inside `timestamps[0] + windowMs`
// (1000 + 1000 = 2000), so the preserved lockout is observable in the
// updated `Retry-After` header.
currentTime += 1100;
// Advance time by 1100 ms. This is 100 ms past the rolling-window expiry
// at 2000 and 900 ms before the original lockout expiry at 3000. A
// preserved lockout therefore returns the remaining Retry-After duration.
currentTime += 1100;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/modules/auth/tests/rate-limit.middleware.test.ts` around lines 115 -
121, Correct the comments immediately above the currentTime += 1100 statement:
state that simulated time advances from 1000 to 2100, remains within the lockout
through 3000, and is 100 ms past the rolling-window timestamp expiry at 2000.
Preserve the test code and clarify that these conditions make the preserved
lockout observable via Retry-After.

Comment on lines +417 to +429
const finalize = (manifest) => {
// Atomically replace the live directory with the validated temp dir.
// `rename` is atomic on the same filesystem on POSIX; Windows treats
// it as remove+create which is fine because no other writer holds the
// directory between the `rename` and the next server restart.
try {
if (fs.existsSync(pluginDir)) {
fs.rmSync(pluginDir, { recursive: true, force: true });
}
fs.renameSync(tempDir, pluginDir);
} catch (err) {
cleanupTemp();
return reject(new Error(`Failed to move updated plugin into place: ${err.message}`));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve the live directory until the replacement succeeds.

Lines 423-425 delete pluginDir before Line 426 moves tempDir. If Line 426 fails, cleanupTemp() removes the staged tree and the previous plugin is lost. plugins.service.ts then cannot load the previous manifest during server recovery.

Move the live directory to a sibling backup first. Move tempDir into place next. Restore the backup if that move fails. Delete the backup only after success. Add a regression test that forces the second move to fail.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/modules/plugins/plugin-registry.service.ts` around lines 417 - 429,
Update the finalize flow to preserve pluginDir: move the existing live directory
to a sibling backup, move tempDir into pluginDir, and delete the backup only
after replacement succeeds. If the second move fails, restore the backup to
pluginDir before rejecting and clean up the staged directory without losing the
previous plugin. Add a regression test covering failure of the second move.

Comment on lines +110 to +132
return {
feed(chunk: string): string {
if (!chunk) return '';

buffer += chunk;
if (buffer.length <= maxPrefix) {
// Not enough characters yet for even a full token to exist; hold
// the whole buffer until the next chunk (or close) and emit nothing.
return '';
}

const safeEnd = buffer.length - maxPrefix;
const safeSlice = buffer.slice(0, safeEnd);
buffer = buffer.slice(safeEnd);
return sanitizeGitError(safeSlice, token);
},
flush(): string {
if (!buffer) return '';
// Any remaining buffer at EOF is a half-token at worst; redact it
// wholesale so no credential fragment survives the stream close.
const remainder = buffer;
buffer = '';
return sanitizeGitError(remainder, token);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file outline =="
ast-grep outline server/modules/projects/services/project-clone.service.ts --match CreateStreamingRedactor --view expanded || true

echo
echo "== relevant source lines =="
sed -n '50,150p' server/modules/projects/services/project-clone.service.ts

echo
echo "== tests around redactor =="
sed -n '1,260p' server/modules/projects/tests/project-clone.service.test.ts

echo
echo "== focused search for createStreamingRedactor and sanitizeGitError =="
rg -n "createStreamingRedactor|sanitizeGitError|maxPrefix|token" server/modules/projects/services/project-clone.service.ts server/modules/projects/tests/project-clone.service.test.ts

Repository: siteboon/claudecodeui

Length of output: 16997


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
def createStreamingRedactor(token: str):
    if not token:
        def feed(chunk: str) -> str:
            return chunk
        def flush() -> str:
            return ''
        return feed, flush

    max_prefix = len(token) - 1
    buffer = ''

    def feed(chunk: str) -> str:
        nonlocal buffer
        if not chunk:
            return ''
        buffer += chunk
        if len(buffer) <= max_prefix:
            return ''
        safe_end = len(buffer) - max_prefix
        safe_slice = buffer[:safe_end]
        buffer = buffer[safe_end:]
        return sanitizeGitError(safe_slice, token)

    def flush() -> str:
        nonlocal buffer
        if not buffer:
            return ''
        remainder = buffer
        buffer = ''
        return sanitizeGitError(remainder, token)

    return feed, flush

def sanitizeGitError(message: str, token: str | None) -> str:
    if not message or not token:
        return message
    import re
    escaped_token = re.sub(r'[.*+?^${}()|[\]\\]', lambda m: '\\' + m.group(0), token)
    return re.sub(escaped_token, '***', message)

token = 'ghp_abcdefghijklmnopqrstuvwxyz0123456789'  # length 40
feed, flush = createStreamingRedactor(token)

# Exact reproduction of the comment's boundary shape:
# a complete token ends such that token[1:] remains in buffer after feeding the prefix.
# buffer will be "token[0] token" after adding one char, emit token[0]
# suffix buffer becomes the complete token, then flushed.
events = ["Cloning into 'repo'...\n", "https://github.com/example/repo.git/token=", token[:-1], token[-1], "\n"]
output = []
for event in events:
    out = feed(event)
    if out:
        output.append(out)
flush += flush()
if flush:
    output.append(flush)
progress = ''.join(output)

print("token_length", len(token))
print("maxPrefix", len(token)-1)
print("events", events)
print("all_output_repr", repr(progress))
print("contains_full_token", token in progress)
print("contains_token_prefix", token[:5] in progress)
print("contains_token_suffix", token[-5:] in progress)

# Alternative edge case mentioned: complete token fully inside retained suffix; whole token never passes through feed sanitization.
feed, flush = createStreamingRedactor(token)
events = ["Cloning into 'repo'...\n", "remote: ", token, "\n"]
output = []
for event in events:
    out = feed(event)
    if out:
        output.append(out)
flush += flush()
if flush:
    output.append(flush)
progress = ''.join(output)
print()
print("edge_events", events)
print("edge_all_output_repr", repr(progress))
print("edge_contains_full_token", token in progress)
print("edge_contains_token_prefix", token[:5] in progress)
print("edge_contains_token_suffix", token[-5:] in progress)
PY

Repository: siteboon/claudecodeui

Length of output: 301


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 <<'PY'
def sanitizeGitError(message, token):
    if not message or not token:
        return message
    import re
    escaped_token = re.sub(r'[.*+?^${}()|[\]\\]', lambda m: '\\' + m.group(0), token)
    return re.sub(escaped_token, '***', message)

def createStreamingRedactor(token):
    if not token:
        def feed(chunk):
            return chunk
        def flush():
            return ''
        return feed, flush

    max_prefix = len(token) - 1
    buffer = ''

    def feed(chunk):
        nonlocal buffer
        if not chunk:
            return ''
        buffer += chunk
        if len(buffer) <= max_prefix:
            return ''
        safe_end = len(buffer) - max_prefix
        safe_slice = buffer[:safe_end]
        buffer = buffer[safe_end:]
        return sanitizeGitError(safe_slice, token)

    def flush():
        nonlocal buffer
        if not buffer:
            return ''
        remainder = buffer
        buffer = ''
        return sanitizeGitError(remainder, token)

    return feed, flush

token = 'ghp_abcdefghijklmnopqrstuvwxyz0123456789'
feed, flush = createStreamingRedactor(token)

events = ["Cloning into 'repo'...\n", "https://github.com/example/repo.git/token=", token[:-1], token[-1], "\n"]
output = []
for event in events:
    out = feed(event)
    if out:
        output.append(out)
flush += flush()
if flush:
    output.append(flush)
progress = ''.join(output)

print("token_length", len(token))
print("maxPrefix", len(token)-1)
print("first_case_all_output_repr", repr(progress))
print("first_case_contains_full_token", token in progress)
print("first_case_contains_token_prefix", token[:5] in progress)
print("first_case_contains_token_suffix", token[-5:] in progress)

feed, flush = createStreamingRedactor(token)
events = ["Cloning into 'repo'...\n", "remote: ", token, "\n"]
output = []
for event in events:
    out = feed(event)
    if out:
        output.append(out)
flush += flush()
if flush:
    output.append(flush)
progress = ''.join(output)
print()
print("edge_events", events)
print("edge_all_output_repr", repr(progress))
print("edge_contains_full_token", token in progress)
print("edge_contains_token_prefix", token[:5] in progress)
print("edge_contains_token_suffix", token[-5:] in progress)
PY

Repository: siteboon/claudecodeui

Length of output: 301


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 <<'PY'
def sanitizeGitError(message, token):
    if not message or not token:
        return message
    import re
    escaped_token = re.sub(r'[.*+?^${}()|[\]\\]', lambda m: '\\' + m.group(0), token)
    return re.sub(escaped_token, '***', message)

def createStreamingRedactor(token):
    if not token:
        def feed(chunk):
            return chunk
        def flush():
            return ''
        return feed, flush

    max_prefix = len(token) - 1
    buffer = ''

    def feed(chunk):
        nonlocal buffer
        if not chunk:
            return ''
        buffer += chunk
        if len(buffer) <= max_prefix:
            return ''
        safe_end = len(buffer) - max_prefix
        safe_slice = buffer[:safe_end]
        buffer = buffer[safe_end:]
        return sanitizeGitError(safe_slice, token)

    def flush():
        nonlocal buffer
        if not buffer:
            return ''
        remainder = buffer
        buffer = ''
        return sanitizeGitError(remainder, token)

    return feed, flush

token = 'ghp_abcdefghijklmnopqrstuvwxyz0123456789'
feed, flush = createStreamingRedactor(token)

events = ["Cloning into 'repo'...\n", "https://github.com/example/repo.git/token=", token[:-1], token[-1], "\n"]
output = []
for event in events:
    out = feed(event)
    if out:
        output.append(out)
flush += flush()
if flush:
    output.append(flush)
progress = ''.join(output)

print("token_length", len(token))
print("maxPrefix", len(token)-1)
print("first_case_all_output_repr", repr(progress))
print("first_case_contains_full_token", token in progress)
print("first_case_contains_token_prefix", token[:5] in progress)
print("first_case_contains_token_suffix", token[-5:] in progress)

feed, flush = createStreamingRedactor(token)
events = ["Cloning into 'repo'...\n", "remote: ", token, "\n"]
output = []
for event in events:
    out = feed(event)
    if out:
        output.append(out)
flush += flush()
if flush:
    output.append(flush)
progress = ''.join(output)
print()
print("edge_events", events)
print("edge_all_output_repr", repr(progress))
print("edge_contains_full_token", token in progress)
print("edge_contains_token_prefix", token[:5] in progress)
print("edge_contains_token_suffix", token[-5:] in progress)
PY

Repository: siteboon/claudecodeui

Length of output: 301


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 <<'PY'
__import__('warning').filterwarnings("ignore", category=SyntaxWarning)

def sanitizeGitError(message, token):
    if not message or not token:
        return message
    import re
    escaped_token = re.sub(r'[.*+?^${}()|[\]\\]', lambda m: '\\' + m.group(0), token)
    return re.sub(escaped_token, '***', message)

def createStreamingRedactor(token):
    if not token:
        def feed(chunk):
            return chunk
        def flush():
            return ''
        return feed, flush

    max_prefix = len(token) - 1
    buffer = ''

    def feed(chunk):
        nonlocal buffer
        if not chunk:
            return ''
        buffer += chunk
        if len(buffer) <= max_prefix:
            return ''
        safe_end = len(buffer) - max_prefix
        safe_slice = buffer[:safe_end]
        buffer = buffer[safe_end:]
        return sanitizeGitError(safe_slice, token)

    def flush():
        nonlocal buffer
        if not buffer:
            return ''
        remainder = buffer
        buffer = ''
        return sanitizeGitError(remainder, token)

    return feed, flush

token = 'ghp_abcdefghijklmnopqrstuvwxyz0123456789'
feed_func, flush_func = createStreamingRedactor(token)

events = ["Cloning into 'repo'...\n", "https://github.com/example/repo.git/token=", token[:-1], token[-1], "\n"]
output = []
for event in events:
    out = feed_func(event)
    if out:
        output.append(out)
flush_out = flush_func()
if flush_out:
    output.append(flush_out)
progress = ''.join(output)

print("token_length", len(token))
print("maxPrefix", len(token)-1)
print("first_case_all_output_repr", repr(progress))
print("first_case_contains_full_token", token in progress)
print("first_case_contains_token_prefix", token[:5] in progress)
print("first_case_contains_token_suffix", token[-5:] in progress)

feed_func, flush_func = createStreamingRedactor(token)
events = ["Cloning into 'repo'...\n", "remote: ", token, "\n"]
output = []
for event in events:
    out = feed_func(event)
    if out:
        output.append(out)
flush_out = flush_func()
if flush_out:
    output.append(flush_out)
progress = ''.join(output)
print()
print("edge_events", events)
print("edge_all_output_repr", repr(progress))
print("edge_contains_full_token", token in progress)
print("edge_contains_token_prefix", token[:5] in progress)
print("edge_contains_token_suffix", token[-5:] in progress)
PY

Repository: siteboon/claudecodeui

Length of output: 279


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 <<'PY'
import warnings
warnings.filterwarnings("ignore", category=SyntaxWarning)

def sanitizeGitError(message, token):
    if not message or not token:
        return message
    import re
    escaped_token = re.sub(r'[.*+?^${}()|[\]\\]', lambda m: '\\' + m.group(0), token)
    return re.sub(escaped_token, '***', message)

def createStreamingRedactor(token):
    if not token:
        def feed(chunk):
            return chunk
        def flush():
            return ''
        return feed, flush

    max_prefix = len(token) - 1
    buffer = ''

    def feed(chunk):
        nonlocal buffer
        if not chunk:
            return ''
        buffer += chunk
        if len(buffer) <= max_prefix:
            return ''
        safe_end = len(buffer) - max_prefix
        safe_slice = buffer[:safe_end]
        buffer = buffer[safe_end:]
        return sanitizeGitError(safe_slice, token)

    def flush():
        nonlocal buffer
        if not buffer:
            return ''
        remainder = buffer
        buffer = ''
        return sanitizeGitError(remainder, token)

    return feed, flush

token = 'ghp_abcdefghijklmnopqrstuvwxyz0123456789'
feed_func, flush_func = createStreamingRedactor(token)

events = ["Cloning into 'repo'...\n", "https://github.com/example/repo.git/token=", token[:-1], token[-1], "\n"]
output = []
for event in events:
    out = feed_func(event)
    if out:
        output.append(out)
flush_out = flush_func()
if flush_out:
    output.append(flush_out)
progress = ''.join(output)

print("token_length", len(token))
print("maxPrefix", len(token)-1)
print("first_case_all_output_repr", repr(progress))
print("first_case_contains_full_token", token in progress)
print("first_case_contains_token_prefix", token[:5] in progress)
print("first_case_contains_token_suffix", token[-5:] in progress)

feed_func, flush_func = createStreamingRedactor(token)
events = ["Cloning into 'repo'...\n", "remote: ", token, "\n"]
output = []
for event in events:
    out = feed_func(event)
    if out:
        output.append(out)
flush_out = flush_func()
if flush_out:
    output.append(flush_out)
progress = ''.join(output)
print()
print("edge_events", events)
print("edge_all_output_repr", repr(progress))
print("edge_contains_full_token", token in progress)
print("edge_contains_token_prefix", token[:5] in progress)
print("edge_contains_token_suffix", token[-5:] in progress)
PY

Repository: siteboon/claudecodeui

Length of output: 734


Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor

Reachability: External

Reachability path
● Entry
  server/modules/providers/provider.routes.ts:690
  listener close
│
▼
● Hop
  server/modules/projects/tests/project-clone.service.test.ts
│
▼
● Sink
  server/modules/projects/services/project-clone.service.ts

Do not emit a complete token before the stream ends.

feed can forward the token prefix because sanitizeGitError only replaces an exact match, and flush returns the remaining token suffix unchanged. If Git emits a token wholly within the retained trailing buffer, that full token reaches handlers.onProgress and the clone-progress SSE response.

Buffer until the token could not complete across another chunk, validate the retained suffix as a real token prefix, and redact it in flush.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/modules/projects/services/project-clone.service.ts` around lines 110 -
132, The feed/flush redaction logic must prevent complete credentials from
reaching progress output. Update the stream sanitizer returned by the relevant
clone-progress service method to retain enough trailing data, detect and redact
any full token within that buffer, and only emit a prefix once the token cannot
complete across another chunk; in flush, validate the retained suffix as an
actual token prefix and redact it rather than returning it unchanged.

Comment on lines 318 to +322
gitProcess.stderr?.on('data', (data: Buffer | string) => {
const message = data.toString().trim();
lastError = message;
if (message) {
handlers.onProgress(message);
}
const raw = data.toString();
lastError = raw;
forwardTrimmed(stderrRedactor.feed(raw));
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== module guidelines =="
if [ -f .agents/skills/backend-module-standards/SKILL.md ]; then
  wc -l .agents/skills/backend-module-standards/SKILL.md
  sed -n '1,220p' .agents/skills/backend-module-standards/SKILL.md
else
  echo "missing"
fi

echo "== relevant service sections =="
wc -l server/modules/projects/services/project-clone.service.ts
sed -n '260,380p' server/modules/projects/services/project-clone.service.ts
sed -n '130,170p' server/modules/projects/services/project-clone.service.ts

echo "== tests around clone failure/redactor tokens =="
wc -l server/modules/projects/tests/project-clone.service.test.ts
rg -n "stderr|lastError|sanitizeGitError|resolveCloneFailureMessage|clone.*fail|token|redact|streaming" server/modules/projects/tests/project-clone.service.test.ts server/modules/projects/services/project-clone.service.ts
sed -n '1,260p' server/modules/projects/tests/project-clone.service.test.ts

Repository: siteboon/claudecodeui

Length of output: 25472


Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor

Reachability: External

Reachability path
● Entry
  server/modules/providers/provider.routes.ts:690
  listener close
│
▼
● Hop
  server/modules/projects/tests/project-clone.service.test.ts
│
▼
● Sink
  server/modules/projects/services/project-clone.service.ts

Do not build clone failures from raw stderr chunks.

lastError overwrites with each unredacted stderr chunk. On failure, resolveCloneFailureMessage() can receive the final chunk, pass an incomplete token to sanitizeGitError(), and return a leak through GIT_CLONE_FAILED. Buffer redacted stderr text instead and cover nonzero close with a split-token stderr case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/modules/projects/services/project-clone.service.ts` around lines 318 -
322, Update the stderr handling in the clone process flow around
resolveCloneFailureMessage so lastError accumulates the redacted output from
every chunk instead of overwriting it with raw stderr. Ensure nonzero process
close passes the complete buffered redacted text through sanitizeGitError,
including tokens split across stderr chunks, and add coverage for that
split-token failure case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants